Skip to content

types: make model types support sync AND async methods #347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

jankapunkt
Copy link
Member

Summary

This is proof of concept for further discussion, related to #344
Please add your opinion and review.

Linked issue(s)

#344

Involved parts of the project

Types

Added tests?

OAuth2 standard

Reproduction

Custom IDE related

@z1haze
Copy link

z1haze commented Jun 2, 2025

Thanks for this

@jankapunkt jankapunkt requested a review from Copilot June 2, 2025 17:12
Copilot

This comment was marked as outdated.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@HappyZombies
Copy link
Member

So I have an opinion here, this isn't really about runtime compatibility (which is fine), since the lib treats every model function as async, but about whether the type system should match what the documentation states.

I think really there are 2 possible solutions:

  1. Update the types to string | Promise to match the doc's promise of sync-or-async. (Which is what this CR is doing).

  2. Update the docs to clarify: “You can write synchronous logic, but model methods must return a Promise.”

To me option 2 seems cleaner — the current typings enforce consistent async shape and avoid noisy unions. Since async functions support returning sync values anyway, for example, just wrap it in async? That will always return a promise.

(async () => 'foo')(); // returns a resolved Promise

What do yall think?

@dhensby
Copy link
Contributor

dhensby commented Jul 9, 2025

I agree with @HappyZombies - people can mark their function async and they are compatible with the typing system and the method is still technically sync (but I admit is returning a promise). Generally I'd prefer a narrower well defined interface in TS than one that covers all the technical possibilities of the implementation (a problem our types currently have with the bespoke Falsey type - something I've thought about trying to remove).

If the OP of the discussion doesn't like sticking to the type declaration, then use JS, if they want to use TS they they'll need to adhere to the typings because those are the intended (rather than possible) interfaces

@jankapunkt
Copy link
Member Author

@HappyZombies @dhensby thank you for your input. Since I'm no TypeScript pro I can't really add valuable information. Feel free to close, if you think this is out of scope.

@dhensby dhensby closed this Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants